Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass progress by value when possible, replace optional with null object #340

Closed
wants to merge 2 commits into from

Conversation

bugadani
Copy link
Contributor

Follow-up for #333 that implements my suggestions. The null object allows cleaning up some of the usage sites, and taking by value is something that I hope improves usability.

Replacing all dyn ProgressCallbacks instances is indeed not possible, but it is not a big deal.

@jessebraham please don't feel in any way pressured to accept this. This PR may or may not actually be a good idea and I only intend this as an illustration of what I had in mind. I did not think of it too deeply and I might have missed use cases where the PR might hurt more than help.

@jessebraham
Copy link
Member

Thanks for the PR demonstrating this. I'm not going to say a hard "no" at this point, but currently I think Option is more clear about the intent, personally. I recognize there are pros and cons to each approach, so I think this ends up being a pretty subjective decision either way. I'm going to cut a new release candidate soon, so maybe we can revisit this after that.

@bugadani
Copy link
Contributor Author

bugadani commented Jan 12, 2023

I'm fine with returning to this later, but I want to share my opinion(s) before I forget :) This is related to the optional type, not the value-vs-reference part of the PR. I understand that I might be forcing you into putting more effort into this API than necessary, so definitely feel free to completely disregard me. Still, I write the rest of this comment, just in case.

I understand that there are cases where users might want a runtime optional callback - we can impl<T: ProgressCallbacks> ProgressCallbacks for Option<T> I think to support these use cases, for example.

In my opinion, Option<impl ProgressCallbacks> as a function argument is redundant. It's easier to think about "this function needs an object (that might do nothing)" than "think function maybe needs an object".

In my opinion, the type Option<impl ProgressCallbacks> is more complex than it needs to be.

In my opinion, Option<impl ProgressCallbacks> optimizes for the negative case, i.e. not using progress callbacks. This optimization makes the (in my opinion, more important) use case of actually getting progress information a tiny bit more cumbersome.

In my opinion, if the concern is discoverability of the null implementation, we can support that by documentation. Either on the methods, or on the trait, or both.

Edit: as it turns out, finding the default EspflashProgress is equally difficult, right now as the proposed null object would be.

@jessebraham
Copy link
Member

This is pretty outdated (and we've already bumped the major version) so closing this. Maybe something we can revisit in the future.

@bugadani
Copy link
Contributor Author

In the big picture, it matters fairly little, though looking at the amount of comments I left here I probably felt a bit more connected to the issue in January 😅

@bugadani bugadani deleted the progress branch September 12, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants